feat: add mixed data file version capability - #8580
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
67d1f56 to
901e1e9
Compare
a6324df to
a1ed170
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bda4b4e to
187cc6d
Compare
9ed0a00 to
9e8b45b
Compare
| &Session::default(), | ||
| ) | ||
| .await?; | ||
| ensure_can_write_manifest(&source_manifest)?; |
There was a problem hiding this comment.
Could we move this compatibility check before clone-side effects? Dataset::deep_clone copies every data, deletion, and index file to the target before CommitBuilder reaches this line, and do_commit_new_dataset has already written the destination transaction file. A reader-compatible source with an unknown writer-only flag therefore returns NotSupported only after leaving a complete uncommitted clone in the target. Please preflight the selected source manifest before copying, while retaining this check for commit-time races.
9e8b45b to
1cc5983
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The bit-256 reservation remains coherent, but this revision is not fail-closed at every boundary: refresh can install an unsupported manifest, namespace overwrite validates only after source flags have been discarded, and the existing clone preflight finding still rejects after copy side effects. The rebased Lance test target also no longer compiles.
A viable revision should validate the freshest source manifest before caching, derivation, staging, or clone copying; retain the final commit-time checks for races; and restore the missing test import.
| use crate::session::Session; | ||
| use crate::session::caches::ManifestKey; | ||
| use crate::{Dataset, Error, Result}; | ||
| use lance_table::format::{DataStorageFormat, Fragment}; |
There was a problem hiding this comment.
Removing Fragment breaks the rebased test target: the current base still uses it in install_fragments and test_get_fragment_on_legacy_manifest. Keep the import (or qualify those uses) so the new restore regression and the Lance lib test target compile.
Reproducer
cargo test -p lance test_restore_rejects_unknown_target_flags --lib
Expected: the focused regression runs.
Observed on 1cc598374e7989c7ba38de1b7b5a21c8a8e1b4a2: compilation fails with E0425 at dataset_io.rs:2962, :2999, and :3005 because Fragment is not in scope.
There was a problem hiding this comment.
Fixed in b8c333b: Fragment is back in scope, and the restore regression now compiles and passes.
review_feedback_thread_marker_template:
| read_struct(object_reader.as_ref(), offset).await | ||
| }?; | ||
|
|
||
| validate_paired_feature_flags(&manifest)?; |
There was a problem hiding this comment.
This validation covers load_manifest, but checkout_latest() calls latest_manifest(), which uses raw read_manifest, caches the result, and installs it without validate_paired_feature_flags or can_read_dataset. An already-open client can therefore refresh onto a paired bit-256 manifest and continue under legacy snapshot-wide dispatch. Route latest refresh/cache through the common validated load path, or validate before caching and installing.
Reproducer
I added a focused test that writes version 2 with auto_set_feature_flags: false, sets FLAG_UNKNOWN in both flag words, then calls dataset.checkout_latest() from version 1 and asserts Error::NotSupported.
cargo test -p lance test_checkout_latest_rejects_unknown_reader_flags --lib
Expected: Err(Error::NotSupported { .. }).
Observed on the current head: checkout_latest() returned Ok(()), so unwrap_err() panicked.
There was a problem hiding this comment.
The latest-refresh and cache paths are repaired, but reader admission is still incomplete: DatasetBuilder::with_serialized_manifest installs a decoded manifest without ensure_can_read_manifest. The focused serialized-manifest regression still loads paired unknown flags successfully, so this admission finding remains and has a current successor projection.
review_feedback_thread_marker_template:
| indices: Option<Vec<IndexMetadata>>, | ||
| transaction: Transaction, | ||
| ) -> std::result::Result<(), CommitError> { | ||
| ensure_can_write_manifest(manifest).map_err(CommitError::from)?; |
There was a problem hiding this comment.
This gates the derived manifest after manifest_from_overwrite_transaction has called Manifest::new_from_previous, which preserves only sticky bit 256 and drops every other unknown writer flag. The earlier ensure_writable(dataset.metadata()) guards namespace table metadata, not Lance manifest flags. A reader-compatible manifest with an unknown writer-only bit can therefore be staged, cleared, then pass this check and republish its files as legacy-compatible. Gate dataset.manifest() before staging/derivation, while retaining this sink check for races.
Reproducer
I added a focused test that sets writer bit 1 << 9 on the source manifest, verifies ensure_can_write_manifest(&source) rejects it, derives the overwrite manifest with manifest_from_overwrite_transaction, and requires the same gate to reject the result.
cargo test -p lance-namespace-impls test_manifest_rewrite_preserves_unknown_writer_flags_until_gate --lib
Expected: the derived manifest remains unsupported.
Observed on the current head: the derived manifest had lost the flag, so ensure_can_write_manifest(&derived) returned Ok(()) and unwrap_err() panicked.
There was a problem hiding this comment.
The overwrite path now gates the source before staging, but declare_table still writes .lance-reserved before any ensure_manifest_writable call. In cargo test -p lance-namespace-impls test_declare_table_rejects_unknown_writer_flag_before_marker --lib, the operation returned NotSupported only after the marker existed, so the pre-side-effect namespace admission finding remains.
review_feedback_thread_marker_template:
There was a problem hiding this comment.
The serialized-manifest admission blocker is fixed in 70b0275: preloaded manifests now cross the shared reader gate before installation.
The author accepts operation-wide orphaned staging artifacts as a deferred operational risk. No further change is requested for this pull request.
Summary
Stack
Testing